Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate report with phys2bids outputs #243

Closed
wants to merge 112 commits into from
Closed

Generate report with phys2bids outputs #243

wants to merge 112 commits into from

Conversation

eurunuela
Copy link
Collaborator

Closes #131

Proposed Changes

  • Adds the functionality to generate a report with phys2bids outputs on every call to phys2bids.

@eurunuela eurunuela added BrainHack This issue is suggested for BrainHack participants! Enhancement New feature or request Minormod This PR generally closes an `Enhancement` issue. It increments the minor version (0.+1.0) labels Jun 16, 2020
@eurunuela
Copy link
Collaborator Author

At the moment, and until we close #242, the report only shows the contents of the log file. Once we have the participant files and we restructure the output into folders, the report will show the folder structure and the plots generated by phys2bids.

@smoia let me know if the Brainhack and Enhancement labels should be removed.

@codecov
Copy link

codecov bot commented Jun 16, 2020

Codecov Report

Merging #243 (dbdf365) into master (e50e3cd) will increase coverage by 0.34%.
The diff coverage is 98.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
+ Coverage   94.77%   95.12%   +0.34%     
==========================================
  Files           8        9       +1     
  Lines         862      944      +82     
==========================================
+ Hits          817      898      +81     
- Misses         45       46       +1     
Impacted Files Coverage Δ
phys2bids/reporting/html_report.py 98.71% <98.71%> (ø)
phys2bids/cli/run.py 96.66% <100.00%> (+0.11%) ⬆️
phys2bids/phys2bids.py 88.33% <100.00%> (+0.19%) ⬆️

@smoia smoia removed BrainHack This issue is suggested for BrainHack participants! Enhancement New feature or request labels Jun 16, 2020
@62442katieb
Copy link
Contributor

62442katieb commented Jun 16, 2020

Hi! 👋 I'd like to help with report generating and I'm curious about what you all would ultimately like the report to look like/contain.

At the moment, and until we close #242, the report only shows the contents of the log file. Once we have the participant files and we restructure the output into folders, the report will show the folder structure and the plots generated by phys2bids.

Based on this comment, report would contain folder structure + plots? Dynamic plots?! ( 😄)

Let me know! (I know it's late, though, no rush)

@eurunuela
Copy link
Collaborator Author

eurunuela commented Jun 16, 2020

Hey Katie! Thanks for collaborating with us!

Ideally it should contain the folder structure generated by physbids, as well as the channel plot (if requested by the user) and the trigger plot. That’s my opinion though.

@smoia anything else you think we should cover?

@smoia
Copy link
Member

smoia commented Jun 17, 2020

... Did anyone say "dynamic"?

If you want to add a bit of bokeh magic, please do so!
The channel plot, if interactive, could become a quality check!

@eurunuela eurunuela reopened this Jun 18, 2020
@smoia smoia assigned vinferrer and unassigned smoia Apr 15, 2021
@eurunuela
Copy link
Collaborator Author

We have to get this merged in before ISMRM.

Any idea why the tests are failing @tsalo ? Afaik tmp_path_factory.getbasetemp() should already create the folder where the data is stored. I don't know what else could be raising this error. When I use a breakpoint first thing in the test, I do not see this error. I only see it when run with no breakpoints.

@eurunuela
Copy link
Collaborator Author

Ok, it looks like the tests pass again. Good catch @vinferrer !

@62442katieb @smoia @vinferrer @tsalo @RayStick the PR is ready for review now!

@eurunuela
Copy link
Collaborator Author

Looks like we figured out what was going on with the logger and the Windows tests. Thank you @vinferrer for your help.

Apparently, the log file generated by test_integration_acq was being read by test_integration_heuristics because it was the first tsv file in the directory. When phys2bids is run through Python and not the terminal, no logs are generated on Linux but apparently, an empty log file is generated on Windows. This empty log file was being read by the test, which caused the errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Minormod This PR generally closes an `Enhancement` issue. It increments the minor version (0.+1.0) Urgent If you don't know where to start, start here!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate a report with all the outputs
5 participants